Skip to content

Simplify qemu shutdown script? #381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

podocarp
Copy link

@podocarp podocarp commented Jul 17, 2025

https://qemu-project.gitlab.io/qemu/interop/qemu-qmp-ref.html#command-QMP-machine.system_powerdown

I'm assuming this is doing an ACPI shutdown request based on how they describe it. But that should be good enough right? For me the current keystroke sequence doesn't even work.

Also I can't help but to modify the close by balloon script as well

  • Don't do anything if balloon is off
  • No need to query capabilities if tail is just gonna strip it away... user can't see it anyways
  • Throw if socket isn't defined as well

@podocarp podocarp marked this pull request as draft July 17, 2025 11:55
@podocarp podocarp marked this pull request as ready for review July 20, 2025 01:40
@podocarp
Copy link
Author

Possible extension: we could do a sleep x in the script and send another kill request to force shutdown? This seems to just be an acpi shutdown which doesn't work if the guest is stuck or not booted up yet.

@astro
Copy link
Collaborator

astro commented Jul 20, 2025

Possible extension: we could do a sleep x in the script and send another kill request to force shutdown? This seems to just be an acpi shutdown which doesn't work if the guest is stuck or not booted up yet.

When run in a systemd service, systemd will kill after timeout already.

cat
) | \
${pkgs.socat}/bin/socat STDIO UNIX:${socket},shut-none
''
else throw "Cannot shutdown without socket";

setBalloonScript =
if socket != null
if socket != null && balloon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line shoud be changed in the other commit which probably should also be an extra commit.

@SuperSandro2000
Copy link
Contributor

A guest may or may not respond to this command. This command returning does not indicate that a guest has accepted the request or that it has shut down. Many guests will respond to this command by prompting the user in some way.

That doesn't read promissing from the docs. We never really want this.

@@ -331,7 +331,7 @@ lib.warnIf (mem == 2048) ''
then
''
(
${writeQmp { execute = "qmp_capabilities"; }}
${writeQmp { execute = "query-commands"; }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your motivation to change this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below, I did not know this was required.

Comment on lines +344 to +347
if balloon
then
if socket != null then
''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change this again, we can drop the change in the last commit, to make the history cleaner.

Comment on lines -348 to -352
${writeQmp { execute = "qmp_capabilities"; }}
${writeQmp { execute = "balloon"; arguments.value = 987; }}
) | sed -e s/987/$VALUE/ | \
${pkgs.socat}/bin/socat STDIO UNIX:${socket},shut-none | \
tail -n 1 | \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you test that this did not break anything?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because I removed the echo capabilities line, which doesn't do anything because it gets tailed away. That line doesn't seem to do anything at all because the user won't be able to see the output.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry I just read the docs; I did not realise you had to execute capabilities to establish a connection, what a strange requirement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants